-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[performance] optimize ES|QL async polling #243104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[performance] optimize ES|QL async polling #243104
Conversation
7dd46a3 to
aba08ef
Compare
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
|
|
||
| let optimizedParams = request.params; | ||
|
|
||
| if (strategy === ESQL_ASYNC_SEARCH_STRATEGY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukasolson curious to hear your thoughts on treating the strategies differently here. Another option would be to add a new optimizeParamsForPoll method to the strategies themselves which would keep the logic together with the strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need anything inside params if we're polling? Could we do something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a slight variant of your patch. Let's see what CI does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passed!
a368263 to
b99c505
Compare
💚 Build Succeeded
Metrics [docs]Page load bundle
History
|
lukasolson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This reverts commit a72addd.
Checking for build failures seen in https://buildkite.com/elastic/kibana-on-merge/builds/82173. May be unrelated, failing tests were not propagating previously - see #243499
…lastic#243586) Checking for build failures seen in https://buildkite.com/elastic/kibana-on-merge/builds/82173. May be unrelated, failing tests were not propagating previously - see elastic#243499
Summary
Fix #242601